Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce verbosity with modern idioms #2

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Reduce verbosity with modern idioms #2

merged 6 commits into from
Feb 22, 2024

Conversation

nickpdemarco
Copy link
Member

@nickpdemarco nickpdemarco commented Feb 22, 2024

  • using ctad to reduce a bit of unnecessary explicitness when constructing new chains
  • renamed namespace chain to namespace chains in order to achieve the above (there was ambiguity when stating chain::chain{...})
  • use a recursive lambda to make result_type_helper a little less verbose
  • added template <class... Args> result_type... and made result_type_helper consteval

It seems the auto clang-format also reformatted a few files.

Copy link

codecov bot commented Feb 22, 2024

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@nickpdemarco nickpdemarco changed the title Use CTAD and deduction guides to reduce verbosity in some places. Reduce verbosity with modern idioms Feb 22, 2024
@@ -101,50 +100,53 @@ using segment_result_type =
simplify this code by handing the multi-argument case earlier (somehow).
*/

#define STLAB_FWD(x) std::forward<decltype(x)>(x)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know where this goes long term, but I feel strongly we need it somewhere. If I see another std::forward<decltype(args)>(args)... I may weep.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be CHAINS_FORWARD() and would go in <chains/utility.hpp> (same header name that contains std::forward<> - not a general dumping ground of utilities).

return std::move(_segment).invoke(receiver, STLAB_FWD(args)...);
};
} else {
return [receiver, _segment = STLAB_FWD(first).append(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the auto formatting is not helping readability here.

Copy link
Member

@sean-parent sean-parent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comment, but approved.

@@ -6,7 +6,7 @@
#ifndef CHAIN_TUPLE_HPP
#define CHAIN_TUPLE_HPP

namespace chain::inline v0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is okay - I wanted to get rid of the plural for the project. Other non-chain suggestions for library and namespace names?

@@ -6,9 +6,9 @@
#include <random>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to just strip all the example files (I've been removing them one by one) instead of reformatting them.

@@ -101,50 +100,53 @@ using segment_result_type =
simplify this code by handing the multi-argument case earlier (somehow).
*/

#define STLAB_FWD(x) std::forward<decltype(x)>(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be CHAINS_FORWARD() and would go in <chains/utility.hpp> (same header name that contains std::forward<> - not a general dumping ground of utilities).

@sean-parent sean-parent merged commit ff26876 into main Feb 22, 2024
@sean-parent sean-parent deleted the npd/ctad branch February 22, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants